Refactor: process_instruction() by Lichtso · Pull Request #20448 · solana-labs/solana

Lichtso · 2021-10-05T20:21:31Z

Problem

The runtime currently uses InvokeContext::remove_first_keyed_account() to pop off the front of the program KeyedAccounts of each invoke stack frame. That necessitates the invoke stack frame to be writable and we want to avoid that in ABIv2 (#19191). Because, if the invoke stack frames are immutable, they can be shared as read-only between runtime, loaders and programs alike with less internal validation checks.

Summary of Changes

Replaces InvokeContext::remove_first_keyed_account() by introducing a num_program_accounts parameter (which is acting as a cursor) to process_instruction().
Also, augments mock ups and tests with the missing beginning of the program chain which was not simulated until now.

Fixes #

codecov · 2021-10-06T19:31:59Z

Codecov Report

Merging #20448 (579579a) into master (258c3bc) will decrease coverage by 0.2%.
The diff coverage is 90.2%.

@@            Coverage Diff            @@
##           master   #20448     +/-   ##
=========================================
- Coverage    82.3%    82.1%   -0.3%     
=========================================
  Files         497      492      -5     
  Lines      136959   137083    +124     
=========================================
- Hits       112854   112673    -181     
- Misses      24105    24410    +305

jackcmay · 2021-10-06T22:50:56Z

I see the assert above but maybe its clearer to just pass the skip variable through?

jackcmay · 2021-10-06T23:26:30Z

Any reason this isn't a structure to help with readability?

jackcmay · 2021-10-06T23:28:26Z

tuple indexing, yuck

jackcmay · 2021-10-06T23:32:11Z

Why are these now tuples and not KeyedAccounts?

jackcmay · 2021-10-06T23:34:59Z

These magic numbers are confusing and there is not info here to indicate what they are based on. Also, is _skip_program_accounts == 1 here, if so, why not base on that?

You are right that we should document what keyed_account_at_index() is based / operating on.
Because now, it is always the beginning of the program chain, followed by the instruction accounts.

I also thought about parametrizing keyed_account_at_index() with skip_program_accounts.
However, that would introduce yet another method like keyed_account_at_base_and_index(keyed_accounts, skip_program_accounts, index), or keyed_account_at_index(keyed_accounts, skip_program_accounts + index) which is getting very long and will be short-lived as it is replaced by the ABIv2 account access pattern.

Lichtso · 2021-10-07T07:37:06Z

About the ugly KeyedAccountTuple:
Some of the tests already used create_keyed_accounts_unified() and that is the format the parameter expects.

I made all the tests to have the wrapper process_instruction() around super::process_instruction() which adds the missing processor_account in front of the program chain and passes the tuples to create_keyed_accounts_unified().

Now, why switch some of the test to create_keyed_accounts_unified() instead of switching the rest to KeyedAccount::new_***()? Because the distinction between the KeyedAccount struct and Rc<RefCell<AccountSharedData>> will be replaced in ABIv2 by an unified access pattern. But I haven't come up with a nice solution to the create_keyed_accounts_unified() parameter for tests and mock ups yet.

…ers.

…programs.

jackcmay · 2021-10-07T23:02:38Z

Name these or at leat provide a comment describing why they are the value that they are

jackcmay

Look good, just one remaining nit about documenting a couple of magic numbers

Pull request has been modified.

jstarry · 2021-11-18T14:44:40Z

    instruction_data: &[u8],
    invoke_context: &mut dyn InvokeContext,
 ) -> Result<(), InstructionError> {
+    debug_assert_eq!(first_instruction_account, 1);


@Lichtso it's possible to hit this debug assert with the tx I described here: https://github.com/solana-labs/solana/pull/21238/files?diff=split&w=0#r752263887

* Adds first_instruction_account parameter to process_instruction(). * Removes InvokeContext::remove_first_keyed_account() from all BPF loaders. * Removes InvokeContext::remove_first_keyed_account() from all builtin programs. * Removes InvokeContext::remove_first_keyed_account() from all mock ups. * Deprecates InvokeContext::remove_first_keyed_account(). * Documents index base of keyed_account_at_index(). * Adds dynamic offset to call sites of "keyed_account_at_index()".

This reverts commit 8b7da50.

vercel Bot temporarily deployed to Preview October 5, 2021 20:21 Inactive

Lichtso force-pushed the refactor/process_instruction branch from b051b76 to a851842 Compare October 5, 2021 20:31

vercel Bot temporarily deployed to Preview October 5, 2021 20:31 Inactive

digidawww approved these changes Oct 5, 2021

View reviewed changes

Lichtso force-pushed the refactor/process_instruction branch from a851842 to fe09137 Compare October 6, 2021 08:07

vercel Bot temporarily deployed to Preview October 6, 2021 08:07 Inactive

Lichtso changed the title ~~Refactor: process_Instruction()~~ Refactor: process_instruction() Oct 6, 2021

Lichtso force-pushed the refactor/process_instruction branch from fe09137 to 4e29b54 Compare October 6, 2021 11:33

vercel Bot temporarily deployed to Preview October 6, 2021 11:33 Inactive

Lichtso force-pushed the refactor/process_instruction branch 2 times, most recently from 17fe3c1 to 05cce99 Compare October 6, 2021 15:35

vercel Bot temporarily deployed to Preview October 6, 2021 15:35 Inactive

Lichtso force-pushed the refactor/process_instruction branch from 05cce99 to cf86549 Compare October 6, 2021 17:22

vercel Bot temporarily deployed to Preview October 6, 2021 17:24 Inactive